-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
various updates (#23) #59
various updates (#23) #59
Conversation
…porting in Report Portal (addresses load issues)
updating parallel run
… folder creation tracking lock file
Continues work and fixes issues with Cucumber parallel executions
Feature/cucumber parallel
small refactoring
@abotalov is it good for merge? |
@DzmitryHumianiuk It still requires updates. Also I've reviewed only some small parts of this PR. I'd prefer if parts I added comments for were addressed before continuing a review. |
@abotalov thank you for comment |
@abotalov your turn |
|
||
set_parallel_tests_vars | ||
|
||
if ParallelTests.first_process? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not fixed.
ParallelTests and Sys::ProcTable are not in Gemfile, they are not required dependencies. A user should be able to use the gem without having to install these dependencies.
If user doesn't specify that they want to run tests in parallel, parallel_tests
and sys/proctable
should not be required and used.
README.md
Outdated
@@ -54,6 +60,8 @@ Supported settings: | |||
- launch_id - id of previously created launch (to be used if formatter_modes contains attach_to_launch) | |||
- file_with_launch_id - path to file with id of launch (to be used if formatter_modes contains attach_to_launch) | |||
- disable_ssl_verification - set to true to disable SSL verification on connect to ReportPortal (potential security hole!). Set `disable_ssl_verification` to `true` if you see the following error: | |||
- launch_uuid - when formatter_mode is `attach_to_launch`, launch_uuid will be used to create uniq report group(tmp dir should be shared across all lunches) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Put a space before the opening parenthesis -
report group (tmp dir should be shared across all lunches)
lunches
->launches
formatter_mode is `attach_to_launch`
->formatter_modes contains `attach_to_launch`
I don't see a difference between launch_id
and launch_uuid
. launch_id
is also a UUID of the launch to be used for attach_to_launch
mode. If launch_id
is provided by the user, reporting should happen to that launch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lunch_id is coming from the report portal and forcing you to create lunch beforehand, with luanch_uuid, there is no need to create lunch beforehand. I'm not using the rake and not familiar with it so we have different approaches to solve the same problem.
|
||
* With Cucumber and parallel_tests gem: | ||
|
||
```parallel_cucumber <some options> -o '<some other options> -f ReportPortal::Cucumber::ParallelFormatter'``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be good to have an example for parallel_cucumber to show that the gem supports parallel_tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're missing the point, it's single formatted for both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that it's a single formatter. But the potential user looking at README should also be aware that agent works when tests are run in parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are more than welcome to contribute.
| attach_to_launch | Do not create a new launch but add executing features/scenarios to an existing launch. Use launch_id or file_with_launch_id settings to configure that. If they are not present client will check rp_launch_id.tmp in `Dir.tmpdir`) | ||
| use_same_thread_for_reporting | Send reporting commands in the same main thread used for running tests. This mode is useful for debugging this Report Portal client. It changes default behavior to send commands in the separate thread. Default behavior is there not to slow test execution. | | ||
| skip_reporting_hierarchy | Do not create items for folders and feature files | | ||
<table><thead><tr><th>Name</th><th>Purpose</th></tr></thead> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is rendered with horizontal scrolling in Github. So a text becomes harder to read in comparison to the current table rendered using Github flavored markdown.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.github.com/gfm/#html-blocks - not sure what you are asking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you can see on the screenshot you attached, not all text in column "Purpose" is visible. There is a horizontal scrolling (at least in Chrome).
https://github.com/reportportal/agent-ruby/blob/7be5353c31be361a2a73b93b83f8e4dffacd8c1b/README.md
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, but I don't see what is the issue here. If you know how to do multiple lines in a simple table block let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previous formatting looked better in the browser IMO. Could you use it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is subjective. Put everything in a single paragraph, and it will not look good in the browser IMO. Hense I had to change it to be more presentable.
@@ -7,12 +7,10 @@ module Cucumber | |||
class Formatter | |||
# @api private | |||
def initialize(config) | |||
ENV['REPORT_PORTAL_USED'] = 'true' | |||
|
|||
@logger = Logger.new(config.out_stream) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logger
is not loaded by Ruby itself. So there should be a require statement before it's first used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require 'logger' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require_relative '../logging/logger' |
* Fix sending plain text to attachment * fixing layout errors Fix sending plain text to attachment removing trailing whitespace * adding gemspec dependency * updating readme * fixing gemspec, code formatting * fixing typo, gemspec update * fixing issue with formatter when file with launch id is provided * fixing typos * fixing typos * fixing typos
It's been a while since PR was open, I have been moved to the different project so if you guys want to merge these changes. PR to my branch is welcome to ensure that it works in our different use cases of the same library. It's been working fine for us so far. All the checks are passing so I don't see any issues. If you guys want to be picky to the cosmetics of the readme or whatnot, then you can commit changes to make it what you desire. I have no problem using the fork of this project but wanted to contribute back as I like the idea of the report portal. And If you have time for #60(which solves nothing from my opinion), there should not have been a problem finishing this PR as well in a timely fashion. |
…iday Feature/cucumber parrallel fariday
splitting client to stand alone class, need to add error handling
fixed issues that were introduced after rebasing, using persistent connection now |
@abotalov are we good to go here? |
The scope of this PR is too large. It makes a number of changes in public API, fixes some bugs, adds some features, does a lot of refactoring. So it's hard to review. I'd prefer to just close it. |
Set description to cmd args of parallel_tests process
Fix matcher to detect when start time needs to be updated
Clean up formatting in report.rb
Implement a folder creation lock file to address duplicate folders being created
Delay each process by its ID number to create a staggered start to reporting in Report Portal (addresses load issues)
Formatting and cleanup in parallel_report.rb
Use temporary directory for tracking folder creation
Add commenting around why we are delaying threads
Make sure to use a "full path" for folder creation tracking
fixing report description when run in parallel
Fix Cucumber parallel formatter
Fix parallel_tests version and possible endless loop
Add debug logging to debug some issue
Add more debug logging
Remove retrying of requests to Report Portal
Set last_used_time to a time of the parent item
Set description to cmd args of parallel_tests process
Fix URLs to include required "filter.eq.launch" parameter
Fix matcher to detect when start time needs to be updated
Clean up formatting in report.rb
Implement a folder creation lock file to address duplicate folders being created
Delay each process by its ID number to create a staggered start to reporting in Report Portal (addresses load issues)
fixing report description when run in parallel
using single formatter for both parallel_cucumber and cucumber
adding logger for report portal
updating readme and adding settings for logging level
updating how response processing is handling
fixing issue with child process not closing the folder's items
fixing value for time, removing cucumber IO as it's no longer needed
adding patch for multipart
updating version